Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[sock-utils]: Add initial support for socket helpers #671

Merged
merged 3 commits into from
Nov 11, 2024
Merged

[sock-utils]: Add initial support for socket helpers #671

merged 3 commits into from
Nov 11, 2024

Conversation

david-cermak
Copy link
Collaborator

@david-cermak david-cermak commented Oct 8, 2024 •
edited
Loading

  • initial support for socket helper library
  • useful for porting linux based libraries where performance and memory is not an issue

Supports:

API Description Limitations
ifaddrs() Uses esp_netif internally Only supports IPv4 addresses
socketpair() Implements using two lwIP loopback stream sockets Only supports IPv4 sockets
pipe() Wraps socketpair() Utilizes bidirectional sockets instead of pipes
getnameinfo() Uses lwIP's inet_ntop() Supports only IPv4, and only NI_NUMERICHOST and NI_NUMERICSERV flags
gai_strerror() Simple itoa-like implementation Only returns the numeric error code as a string

@david-cermak david-cermak self-assigned this Oct 8, 2024
@david-cermak david-cermak force-pushed the feat/sock_utils branch 2 times, most recently from c99baac to 743a643 Compare October 14, 2024 11:49
@david-cermak david-cermak force-pushed the feat/sock_utils branch 4 times, most recently from 8213d5f to 8e3ebb9 Compare October 14, 2024 13:15
@david-cermak david-cermak marked this pull request as ready for review October 14, 2024 13:16
@david-cermak david-cermak changed the title feat(sockutls): Add initial support for socket helpers [sock-utils]: Add initial support for socket helpers Oct 14, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 14, 2024
View reviewed changes
components/sock_utils/src/gai_strerror.c Fixed Show resolved Hide resolved
components/sock_utils/src/getnameinfo.c Fixed Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Fixed Show resolved Hide resolved
components/sock_utils/src/socketpair.c Fixed Show fixed Hide fixed
david-cermak
david-cermak commented Oct 14, 2024
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
This was referenced Oct 30, 2024
Open
Closed
kostaond
kostaond reviewed Oct 30, 2024
View reviewed changes
components/sock_utils/include/ifaddrs.h Outdated Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Outdated Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Outdated Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Outdated Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Outdated Show resolved Hide resolved
components/sock_utils/src/ifaddrs.c Outdated Show resolved Hide resolved
alisitsyn
alisitsyn reviewed Oct 31, 2024
View reviewed changes
components/sock_utils/include/ifaddrs.h
struct ifaddrs {
struct ifaddrs *ifa_next; /* Next item in list */
char *ifa_name; /* Name of interface */
struct sockaddr *ifa_addr; /* Address of interface */
Copy link
Collaborator

@alisitsyn alisitsyn Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the ifa_data - address-family- specific data in spite that support for other family is not implemented? It is better to keep it for compatibility reason with linux API and set it to NULL. Consider to add other fields for the same reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the structure minimal, to contain only the fields which are supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll reconsider

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the structure definition in the original shape, to keep it minimal.

@david-cermak
Copy link
Collaborator Author

Name of the component is misleading, consider:

  • porting
  • linux
    help people find this component

@david-cermak
Copy link
Collaborator Author

Document how and where are the prototypes defined

@david-cermak
Copy link
Collaborator Author

Document how and where are the prototypes defined

Add to the IDF hints -- if we get pipe() linkage error -> please install sock-utils (or whatever w'll call it)

alisitsyn
alisitsyn reviewed Oct 31, 2024
View reviewed changes
components/sock_utils/include/netdb_macros.h
*/
#pragma once

#ifndef NI_NUMERICHOST
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Can the 'NI_MAXHOST', 'NI_MAXSERV' be added here to specify the maximum sizes required for the buffers?
This will allow to avoid magic numbers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will use these number when allocating buffers in tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used NI_MAXSERV for buffer size in tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately I had to revert this change to pass the CI.
This is defined in newlib headers in IDF, and newlib is not supported on linux target.
I can use some system header (which works on my machine), but wouldn't work on build machine.

@david-cermak david-cermak force-pushed the feat/sock_utils branch 7 times, most recently from 4a89e8c to 874b955 Compare November 1, 2024 11:16
@david-cermak
Copy link
Collaborator Author

Name of the component is misleading, consider:

  • porting
  • linux
    help people find this component

other ideas (since we have the linux target):

  • net_linux_compat
  • linux_sock_compat

alisitsyn
alisitsyn approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@alisitsyn alisitsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

kostaond
kostaond approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@kostaond kostaond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@igrr igrr mentioned this pull request Nov 11, 2024
Closed
3 tasks
@david-cermak david-cermak merged commit 67191f3 into espressif:master Nov 11, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@euripedesrocha euripedesrocha euripedesrocha left review comments

@espressif-abhikroy espressif-abhikroy espressif-abhikroy approved these changes

@kostaond kostaond kostaond approved these changes

@alisitsyn alisitsyn alisitsyn approved these changes

@bogdankolendovskyy bogdankolendovskyy Awaiting requested review from bogdankolendovskyy

Assignees

@david-cermak david-cermak

Labels
sock_utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@david-cermak @euripedesrocha @alisitsyn @kostaond @espressif-abhikroy